feat: add flag to skip unknown fields instead of error #36
Conversation
There was a problem hiding this comment.
Thank you for your work on this, this will be an awesome feature to have 👍
I've left some comments around testing, I think it would be good to avoid test duplication, and making the build.rs even more complex than they already are. I left some ideas on how we might simplify this. I would also like to see some tests of the unknown fields behaviour.
If you could also please sign the CLA, this will allow me to get this in once ready 😄
|
Hi, thanks for looking at this and the feedback! Sorry for the late reply, got emergency surgery last week :( so didn't took a look at this, I'm already feeling better, although my capacity will be a little limited, so I may move slowly with this one, but hopefully will have all the comments addressed soon. Thanks! |
|
Sorry for the delay, I'm now recovered and with enough time to look at it, also, if you want to take a stab to removing the duplication from the test I'm ok with that. BTW, I checked and was added to the coorporate CLA for Amazon-influxData last week got the confirmation from the influxData side, not sure if I need to provide anything else, let me know. |
|
Glad to hear your feeling better, sorry it's taken so long to get back to you, been a bit swamped 😅 I'll take a stab at reducing the test duplication |
tustvold
force-pushed
the
main
branch
2 times, most recently
from
141970d to
d6454f8
Compare
tustvold
changed the title
This is mostly required for forward compatibility, when new fields are added, any old instance will fail to deserialize a message serialized with a newer version. This is also aligned with prost that skips any unknown field too. I added a flag to to the builder, so this behavior is opt-in, keeping the old behavior (error out for unknown fields) as default.
What?
Allow skipping unknown fields when deserializing.
I added a flag to to the builder, so this behavior is opt-in, keeping
the old behavior (error out for unknown fields) as default.
Why?
This is mostly required for forward compatibility, when new fields
are added, any old instance will fail to deserialize a message
serialized with a newer version. This is also aligned with prost that
skips any unknown field too.
How?
Added a special variant
GeneratedField::__SkipField__that is used every time an unknown field is encountered, which then is used to consume and ignore the value of the field. This will be added only if theskip_unknown_fieldsflag is set.To set the flag, a new
Builder::skip_unknown_fieldsfunction was added.Other notes
I generated the code for the tests twice, to test the skip functionality properly. This may not be the best way as it may not scale, but didn't wanted to impose any particular style on that one. I'm totally open to work it out.